Copy tenant with Short URL #1804

cwperks · 2024-02-23T17:03:49Z

Description

This PR fixes the bug described in #1769 where copying the short url is not including tenant information as a url param.

Issues Resolved

[BUG] Copying Short URL is not adding the security_tenant query parameter #1769

Testing

Add tests for shared links when multi-tenancy is enabled opensearch-dashboards-functional-test#1128

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>

derek-ho · 2024-02-23T17:27:14Z

Blocked on: #1783, since github actions were moved out of security repo

derek-ho · 2024-02-26T21:03:16Z

@cwperks can you run yarn lint:es --fix?

derek-ho · 2024-02-26T21:04:10Z

Will add this linting as a precommit hook as a part of this issue for the future: #1805

Signed-off-by: Craig Perkins <[email protected]>

…ks/security-dashboards-plugin into copy-short-url-with-tenant

codecov · 2024-02-26T21:18:57Z

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 67.38%. Comparing base (9516bea) to head (1d84775).

Files	Patch %	Lines
public/services/shared-link.ts	76.92%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1804      +/-   ##
==========================================
- Coverage   67.41%   67.38%   -0.03%     
==========================================
  Files          94       94              
  Lines        2409     2410       +1     
  Branches      321      320       -1     
==========================================
  Hits         1624     1624              
- Misses        707      708       +1     
  Partials       78       78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

public/services/shared-link.ts

cwperks · 2024-02-26T22:23:59Z

Opened PR in FTR repo that contains tests for this: opensearch-project/opensearch-dashboards-functional-test#1128

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-02-27T02:01:15Z

Install Dashboards with Plugin via Binary is failing with:

 FATAL  Error: Cannot find module './authentication_methods_registry.mock'
Require stack:
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/auth_registry/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/plugin.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/plugin.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/discovery/plugins_discovery.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/discovery/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/plugins_service.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/serve/serve.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/cli.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/dist.js

Example run: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/8058358488/job/22011023503?pr=1804

derek-ho · 2024-02-27T14:48:05Z

Install Dashboards with Plugin via Binary is failing with:

 FATAL  Error: Cannot find module './authentication_methods_registry.mock'
Require stack:
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/auth_registry/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/plugin.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/plugins/data_source/server/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/plugin.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/discovery/plugins_discovery.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/discovery/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/plugins_service.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/plugins/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/core/server/index.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/serve/serve.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/cli.js
- /home/runner/work/security-dashboards-plugin/security-dashboards-plugin/opensearch-dashboards-3.0.0-linux-x64/src/cli/dist.js

Example run: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/8058358488/job/22011023503?pr=1804

I was seeing this too across a few repos I added the workflow for. It's coming from core I think. I created an issue: opensearch-project/OpenSearch-Dashboards#5952. This workflow is also a good way to catch issues in core 😉

Signed-off-by: Craig Perkins <[email protected]>

…ks/security-dashboards-plugin into copy-short-url-with-tenant

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura

Changes look good to me. Thank you for fixing this bug @cwperks !

derek-ho

Changes look good, going to check out your branch and verify it works as expected

derek-ho

For some reason safari is dropping the tenant from the share button, can you take a look?

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-02-29T16:26:20Z

For some reason safari is dropping the tenant from the share button, can you take a look?

@derek-ho Pushed a fix. Safari was having an issue with creating a new range instead of replacing the existing range. I pushed a change to get the range if it exists, modify it and add it back.

public/services/shared-link.ts

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-03-01T15:24:10Z

@derek-ho I pushed one more small change. I noticed after more testing that since fixing the bug on short url, it was finally hitting the block where it would replace the text content and it was not correctly assembling the new url with the protocol and hash as well. The last commit fixes that to assemble the full url and add the security_tenant url param before the hash and after the search params. If no search params exist, the delimiter will be ? since its the first url param. If url params already exist, it will add it on with & as the delimeter.

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-03-01T19:57:39Z

@peternied Sorry for one more push, but I found a way to really simplify the logic with adding the security_tenant as a url param. Instead of re-assembling the url from its constituent components I instead use the URL class from javascript and directly parse the URL string and append a searchParam if its not already there.

This also adds a test for the iFrame logic because the copy link with an iFrame should return an iFrame with an updated src attribute that includes security_tenant as a url param.

Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit c80b64a)

Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit c80b64a) Co-authored-by: Craig Perkins <[email protected]>

Copy tenant with Short URL

cd6d632

Signed-off-by: Craig Perkins <[email protected]>

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners February 23, 2024 17:03

Merge branch 'main' into copy-short-url-with-tenant

6ca78b4

cwperks added 2 commits February 26, 2024 16:09

Run es lint fix

85df25b

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'copy-short-url-with-tenant' of https://github.com/cwper…

4c76663

…ks/security-dashboards-plugin into copy-short-url-with-tenant

peternied reviewed Feb 26, 2024

View reviewed changes

public/services/shared-link.ts Outdated Show resolved Hide resolved

cwperks mentioned this pull request Feb 26, 2024

Add tests for shared links when multi-tenancy is enabled opensearch-project/opensearch-dashboards-functional-test#1128

Closed

1 task

Check for nextUrl

82b476f

Signed-off-by: Craig Perkins <[email protected]>

cwperks added 4 commits February 27, 2024 10:24

Figure out issue with selection

7f87e21

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'copy-short-url-with-tenant' of https://github.com/cwper…

bd05e1e

…ks/security-dashboards-plugin into copy-short-url-with-tenant

Add assertion to unit test

2ebd39c

Signed-off-by: Craig Perkins <[email protected]>

Fix other test

eb0556f

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura previously approved these changes Feb 28, 2024

View reviewed changes

cwperks added the backport 2.x backport to 2.x branch label Feb 28, 2024

derek-ho approved these changes Feb 29, 2024

View reviewed changes

derek-ho requested changes Feb 29, 2024

View reviewed changes

Use existing range

2c93fa1

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed DarshitChanpura’s stale review via 2c93fa1 February 29, 2024 16:24

peternied previously approved these changes Mar 1, 2024

View reviewed changes

DarshitChanpura previously approved these changes Mar 1, 2024

View reviewed changes

public/services/shared-link.ts Show resolved Hide resolved

Include protocol and hash in new value

28198d2

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed stale reviews from DarshitChanpura and peternied via 28198d2 March 1, 2024 15:39

Update unit test

89e4b2c

Signed-off-by: Craig Perkins <[email protected]>

peternied previously approved these changes Mar 1, 2024

View reviewed changes

Simplify logic and add test with iframe

1d84775

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed peternied’s stale review via 1d84775 March 1, 2024 19:55

peternied approved these changes Mar 1, 2024

View reviewed changes

DarshitChanpura approved these changes Mar 2, 2024

View reviewed changes

DarshitChanpura merged commit c80b64a into opensearch-project:main Mar 2, 2024
15 checks passed

opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 2, 2024

Copy tenant with Short URL (#1804)

c1f07aa

Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit c80b64a)

opensearch-trigger-bot bot mentioned this pull request Mar 2, 2024

[Backport 2.x] Copy tenant with Short URL #1812

Merged

derek-ho mentioned this pull request Mar 14, 2024

Rebasing with main (#1828) #1829

Closed

3 tasks

derek-ho mentioned this pull request Mar 27, 2024

[BUG] Copying Short URL is not adding the security_tenant query parameter #1769

Closed

cwperks added the v2.13.0 label Jul 16, 2024

cwperks mentioned this pull request Jul 16, 2024

[BUG] Add security_tenant parameter for short links #1970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy tenant with Short URL #1804

Copy tenant with Short URL #1804

cwperks commented Feb 23, 2024 •

edited

Loading

derek-ho commented Feb 23, 2024

derek-ho commented Feb 26, 2024 •

edited

Loading

derek-ho commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

cwperks commented Feb 26, 2024

cwperks commented Feb 27, 2024

derek-ho commented Feb 27, 2024

DarshitChanpura left a comment

derek-ho left a comment

derek-ho left a comment

cwperks commented Feb 29, 2024

cwperks commented Mar 1, 2024

cwperks commented Mar 1, 2024

Copy tenant with Short URL #1804

Copy tenant with Short URL #1804

Conversation

cwperks commented Feb 23, 2024 • edited Loading

Description

Category

Issues Resolved

Testing

Check List

derek-ho commented Feb 23, 2024

derek-ho commented Feb 26, 2024 • edited Loading

derek-ho commented Feb 26, 2024 • edited Loading

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

cwperks commented Feb 26, 2024

cwperks commented Feb 27, 2024

derek-ho commented Feb 27, 2024

DarshitChanpura left a comment

Choose a reason for hiding this comment

derek-ho left a comment

Choose a reason for hiding this comment

derek-ho left a comment

Choose a reason for hiding this comment

cwperks commented Feb 29, 2024

cwperks commented Mar 1, 2024

cwperks commented Mar 1, 2024

cwperks commented Feb 23, 2024 •

edited

Loading

derek-ho commented Feb 26, 2024 •

edited

Loading

derek-ho commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading